[incubator-kie-drools-5792] [new-parser] improve drools-drl-parser-tests #5855

tkobayas · 2024-04-21T06:30:20Z

Old and new parser coverage using 2 surefire test executions
Fixed Descr common property issue to keep backward compatibility
A few test cases remaining without backward compatibility ("Backward Compatibility Notes" left) because the old parser seems to be wrong.
A few expr test cases remaining without backward compatibility ("Backward Compatibility Notes" left). Error code/message difference comes from antlr4 and 3 difference. Also the new parser ones are better.

Issue:

[new-parser] improve drools-drl-parser-tests to test with old and new parsers #5854

Before PR in `drools-model-codegen`

[ERROR] Tests run: 2434, Failures: 97, Errors: 0, Skipped: 9

After PR in `drools-model-codegen`

[ERROR] Tests run: 2434, Failures: 90, Errors: 0, Skipped: 9

…stsa to test with old and new parsers - Old and new parser coverage using 2 surefire test executions - Fixed Descr common property issues to keep backward compatibility - A few test cases remaining without backward compatibility ("Backward Compatibility Notes" left) because the old parser seems to be wrong. - A few expr test cases remaining without backward compatibility ("Backward Compatibility Notes" left). Error code/message don't seem to be important. Also the new parser ones are better.

tkobayas · 2024-04-21T06:32:13Z

drools-drl/drools-drl-parser-tests/pom.xml

+                            <execution>
+                                <!-- override default-test for new parser -->
+                                <id>default-test</id>
+                                <configuration>
+                                    <systemPropertyVariables>
+                                        <drools.drl.antlr4.parser.enabled>true</drools.drl.antlr4.parser.enabled>
+                                    </systemPropertyVariables>
+                                </configuration>
+                            </execution>
+                            <execution>
+                                <id>old-parser-test</id>
+                                <configuration>
+                                    <systemPropertyVariables>
+                                        <drools.drl.antlr4.parser.enabled>false</drools.drl.antlr4.parser.enabled>
+                                    </systemPropertyVariables>
+                                </configuration>
+                                <goals>
+                                    <goal>test</goal>
+                                </goals>
+                            </execution>


Now we can run old and new parser for the drools-drl-parser-tests

I'm not sure if I understand. Does this run the new tests (that we keep on adding to on this feature branch) against both the new and the old parser?

@yurloc Yes, in drools-drl-parser-tests, surefire runs twice with drools.drl.antlr4.parser.enabled = true and false. You would see:

[INFO] --- surefire:3.1.2:test (default-test) @ drools-drl-parser-tests --- [INFO] Using auto detected provider org.apache.maven.surefire.junitplatform.JUnitPlatformProvider [INFO] [INFO] ------------------------------------------------------- [INFO] T E S T S [INFO] ------------------------------------------------------- [INFO] Running org.drools.drl.parser.antlr4.DRLExprParserTest ... ### parse : ANTLR4_PARSER_ENABLED = true ### parse : ANTLR4_PARSER_ENABLED = true ... [INFO] [INFO] Results: [INFO] [WARNING] Tests run: 351, Failures: 0, Errors: 0, Skipped: 17 [INFO] [INFO] [INFO] --- surefire:3.1.2:test (old-parser-test) @ drools-drl-parser-tests --- [INFO] Using auto detected provider org.apache.maven.surefire.junitplatform.JUnitPlatformProvider [INFO] [INFO] ------------------------------------------------------- [INFO] T E S T S [INFO] ------------------------------------------------------- [INFO] Running org.drools.drl.parser.antlr4.DRLExprParserTest ... ### parse : ANTLR4_PARSER_ENABLED = false ### parse : ANTLR4_PARSER_ENABLED = false ... [INFO] [INFO] Results: [INFO] [WARNING] Tests run: 351, Failures: 0, Errors: 0, Skipped: 17

tkobayas · 2024-04-21T06:39:16Z

...rl/drools-drl-parser-tests/src/test/java/org/drools/drl/parser/antlr4/DRLExprParserTest.java

+        // Backward Compatibility Notes:
+        //   Antlr4 gives a different error code/message from antlr3 for this case.
+        //   Backward compatibility doesn't seem to be required in this case.


I leave Backward Compatibility Notes when test results are different between old and new parser. We can categorize the cases into:

A) The difference comes from Antrl3 and 4, so we cannot change.
B) Old parser seems to be wrong and if we mimic the old behaviour, it would get unnecessarily complex. -> but, of course, we can discuss and look for compromise.

Can you please give us one or more examples of B)? In which cases the old parser is wrong? This would help us to focus this discussion and decide if it makes sense to pollute the new parser to accommodate the weird behaviors of the old one (I hope not).

@mariofusco Sure, commented below. Probably it's easier to look in https://github.com/apache/incubator-kie-drools/pull/5855/files

tkobayas · 2024-04-21T06:44:51Z

drools-drl/drools-drl-parser/src/main/java/org/drools/drl/parser/antlr4/LexerHelper.java

+    /**
+     * Determine if the current token is the end of a RHS DRL by lookahead.
+     * 1. 'end'
+     * 2. skip semi, WS, and comment
+     * 3. next token should be EOF or statement or attribute keyword
+     *
+     * TODO: This method is low-level and may be too complex in order to keep backward compatibility.
+     *       This could be refactored by going back to a parser rather than the lexer island mode.
+     */
+    boolean isRhsDrlEnd() {


isRhsDrlEnd is required because we use lexer island mode and end has a tricky condition (to meet backward compatibility) for a semantic predicate. This method is low-level and too complex. I filed #5856 to investigate a better approach.

tkobayas · 2024-04-21T06:59:21Z

@yurloc @gitgabrio @mariofusco Please review, thanks!

tkobayas · 2024-04-22T07:13:32Z

...rl/drools-drl-parser-tests/src/test/java/org/drools/drl/parser/antlr4/DRLExprParserTest.java

+        // Backward Compatibility Notes:
+        //   Old expr parser (DRL6Expressions) allows this expression because it's too tolerant (fail at runtime anyway).
+        //   Backward compatibility doesn't seem to be required in this case. (But we may align with the old tolerant behavior.)
+        if (DrlParser.ANTLR4_PARSER_ENABLED) {


Backward Compatibility Notes : B) Old parser seems to be wrong

Old parser allows x.int.

tkobayas · 2024-04-22T07:17:47Z

...ols-drl-parser-tests/src/test/java/org/drools/drl/parser/antlr4/DescrCommonPropertyTest.java

+        // Backward Compatibility Notes:
+        //   The old DRL6Parser uses only the attribute value token for common properties (seem to be wrong), so startCharacter and column are different between old and new parser.
+        //   Backward compatibility doesn't seem to be required in this case.


Backward Compatibility Notes : B) Old parser seems to be wrong

For TypeFieldDescr name : String ,
Old parser handles only String for common properties. So startCharacter is the position of S
New parser handles the whole expression (like other Descr), so startCharacter is the position of n

tkobayas · 2024-04-22T07:19:32Z

...ols-drl-parser-tests/src/test/java/org/drools/drl/parser/antlr4/DescrCommonPropertyTest.java

+        // Backward Compatibility Notes:
+        //   The old DRL6Parser uses only the attribute value token for common properties (seem to be wrong), so startCharacter and column are different between old and new parser.
+        //   Backward compatibility doesn't seem to be required in this case. (If do it, the code would be unnecessarily complicated.)


Backward Compatibility Notes : B) Old parser seems to be wrong

For AttributeDescr salience 42 ,
Old parser handles only 42 for common properties. So startCharacter is the position of 4
New parser handles the whole expression (like other Descr), so startCharacter is the position of s

tkobayas · 2024-04-22T07:20:48Z

...ols-drl-parser-tests/src/test/java/org/drools/drl/parser/antlr4/DescrCommonPropertyTest.java

+        // Backward Compatibility Notes:
+        //   The old DRL6Parser doesn't populate common properties of FromDescr (seem to be wrong).
+        //   Backward compatibility doesn't seem to be required in this case.


Backward Compatibility Notes : B) Old parser seems to be wrong

Old parser doesn't populate FromDescr common properties. So all -1.

tkobayas · 2024-04-22T07:21:07Z

...ols-drl-parser-tests/src/test/java/org/drools/drl/parser/antlr4/DescrCommonPropertyTest.java

+        // Backward Compatibility Notes:
+        //   The old DRL6Parser doesn't populate common properties of EntryPointDescr (seem to be wrong).
+        //   Backward compatibility doesn't seem to be required in this case.


Backward Compatibility Notes : B) Old parser seems to be wrong

Old parser doesn't populate EntryPointDescr common properties. So all -1.

tkobayas · 2024-04-22T07:21:29Z

...ols-drl-parser-tests/src/test/java/org/drools/drl/parser/antlr4/DescrCommonPropertyTest.java

+        // Backward Compatibility Notes:
+        //   The old DRL6Parser doesn't populate common properties of WindowReferenceDescr (seem to be wrong).
+        //   Backward compatibility doesn't seem to be required in this case.


Backward Compatibility Notes : B) Old parser seems to be wrong

Old parser doesn't populate WindowReferenceDescr common properties. So all -1.

yurloc

Nice work!

Fixes:

drools-drl/drools-drl-parser/src/main/java/org/drools/drl/parser/antlr4/LexerHelper.java

yurloc · 2024-04-22T20:42:13Z

drools-drl/drools-drl-parser-tests/pom.xml

+                            <execution>
+                                <!-- override default-test for new parser -->
+                                <id>default-test</id>
+                                <configuration>
+                                    <systemPropertyVariables>
+                                        <drools.drl.antlr4.parser.enabled>true</drools.drl.antlr4.parser.enabled>
+                                    </systemPropertyVariables>
+                                </configuration>
+                            </execution>
+                            <execution>
+                                <id>old-parser-test</id>
+                                <configuration>
+                                    <systemPropertyVariables>
+                                        <drools.drl.antlr4.parser.enabled>false</drools.drl.antlr4.parser.enabled>
+                                    </systemPropertyVariables>
+                                </configuration>
+                                <goals>
+                                    <goal>test</goal>
+                                </goals>
+                            </execution>


I'm not sure if I understand. Does this run the new tests (that we keep on adding to on this feature branch) against both the new and the old parser?

...ols-drl-parser-tests/src/test/java/org/drools/drl/parser/antlr4/DescrCommonPropertyTest.java

yurloc · 2024-04-22T21:22:02Z

...ols-drl-parser-tests/src/test/java/org/drools/drl/parser/antlr4/DescrCommonPropertyTest.java

        final RuleDescr rule = pkg.getRules().get(0);
        AndDescr and = rule.getLhs();
-        assertProperties(and, 21, 87, 3, 6, 3, 72);
+        assertProperties(and, 19, 90, 3, 4, 3, 74);


For the record, I checked this one and the new numbers are correct.

Yes, previously, it didn't include surrounding parentheses.

…er/antlr4/LexerHelper.java Co-authored-by: Jiří Locker <[email protected]>

…sts (apache#5855) * [incubator-kie-drools-5854] [new-parser] improve drools-drl-parser-testsa to test with old and new parsers - Old and new parser coverage using 2 surefire test executions - Fixed Descr common property issues to keep backward compatibility - A few test cases remaining without backward compatibility ("Backward Compatibility Notes" left) because the old parser seems to be wrong. - A few expr test cases remaining without backward compatibility ("Backward Compatibility Notes" left). Error code/message don't seem to be important. Also the new parser ones are better. * Update drools-drl/drools-drl-parser/src/main/java/org/drools/drl/parser/antlr4/LexerHelper.java Co-authored-by: Jiří Locker <[email protected]> * - removed unused import --------- Co-authored-by: Jiří Locker <[email protected]>

…sts (#5855) * [incubator-kie-drools-5854] [new-parser] improve drools-drl-parser-testsa to test with old and new parsers - Old and new parser coverage using 2 surefire test executions - Fixed Descr common property issues to keep backward compatibility - A few test cases remaining without backward compatibility ("Backward Compatibility Notes" left) because the old parser seems to be wrong. - A few expr test cases remaining without backward compatibility ("Backward Compatibility Notes" left). Error code/message don't seem to be important. Also the new parser ones are better. * Update drools-drl/drools-drl-parser/src/main/java/org/drools/drl/parser/antlr4/LexerHelper.java Co-authored-by: Jiří Locker <[email protected]> * - removed unused import --------- Co-authored-by: Jiří Locker <[email protected]>

…sts (apache#5855) * [incubator-kie-drools-5854] [new-parser] improve drools-drl-parser-testsa to test with old and new parsers - Old and new parser coverage using 2 surefire test executions - Fixed Descr common property issues to keep backward compatibility - A few test cases remaining without backward compatibility ("Backward Compatibility Notes" left) because the old parser seems to be wrong. - A few expr test cases remaining without backward compatibility ("Backward Compatibility Notes" left). Error code/message don't seem to be important. Also the new parser ones are better. * Update drools-drl/drools-drl-parser/src/main/java/org/drools/drl/parser/antlr4/LexerHelper.java Co-authored-by: Jiří Locker <[email protected]> * - removed unused import --------- Co-authored-by: Jiří Locker <[email protected]>

tkobayas force-pushed the incubator-kie-drools-5854-improve-drools-drl-parser-tests branch from d3ec507 to e75d0d6 Compare April 21, 2024 06:31

tkobayas commented Apr 21, 2024

View reviewed changes

tkobayas requested review from mariofusco and gitgabrio April 21, 2024 06:59

mariofusco approved these changes Apr 22, 2024

View reviewed changes

tkobayas commented Apr 22, 2024

View reviewed changes

yurloc approved these changes Apr 22, 2024

View reviewed changes

tkobayas and others added 2 commits April 23, 2024 15:07

Update drools-drl/drools-drl-parser/src/main/java/org/drools/drl/pars…

fa142c3

…er/antlr4/LexerHelper.java Co-authored-by: Jiří Locker <[email protected]>

- removed unused import

93f38d9

tkobayas merged commit 025ff72 into apache:dev-new-parser Apr 23, 2024
0 of 3 checks passed

This was referenced Apr 25, 2024

[new-parser] end with succeeding rule in the same line fails #5727

Closed

[new-parser] improve drools-drl-parser-tests to test with old and new parsers #5854

Closed

tkobayas mentioned this pull request Jun 7, 2024

[new-parser] Backward compatibility issues #5962

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[incubator-kie-drools-5792] [new-parser] improve drools-drl-parser-tests #5855

[incubator-kie-drools-5792] [new-parser] improve drools-drl-parser-tests #5855

tkobayas commented Apr 21, 2024 •

edited

Loading

tkobayas Apr 21, 2024

yurloc Apr 22, 2024

tkobayas Apr 23, 2024

tkobayas Apr 21, 2024 •

edited

Loading

mariofusco Apr 22, 2024

tkobayas Apr 22, 2024 •

edited

Loading

tkobayas Apr 21, 2024 •

edited

Loading

tkobayas commented Apr 21, 2024

tkobayas Apr 22, 2024 •

edited

Loading

tkobayas Apr 22, 2024

tkobayas Apr 22, 2024

tkobayas Apr 22, 2024 •

edited

Loading

tkobayas Apr 22, 2024 •

edited

Loading

tkobayas Apr 22, 2024 •

edited

Loading

yurloc left a comment

yurloc Apr 22, 2024

yurloc Apr 22, 2024

tkobayas Apr 23, 2024

[incubator-kie-drools-5792] [new-parser] improve drools-drl-parser-tests #5855

[incubator-kie-drools-5792] [new-parser] improve drools-drl-parser-tests #5855

Conversation

tkobayas commented Apr 21, 2024 • edited Loading

Before PR in drools-model-codegen

After PR in drools-model-codegen

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkobayas Apr 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkobayas Apr 22, 2024 • edited Loading

Choose a reason for hiding this comment

tkobayas Apr 21, 2024 • edited Loading

Choose a reason for hiding this comment

tkobayas commented Apr 21, 2024

tkobayas Apr 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkobayas Apr 22, 2024 • edited Loading

Choose a reason for hiding this comment

tkobayas Apr 22, 2024 • edited Loading

Choose a reason for hiding this comment

tkobayas Apr 22, 2024 • edited Loading

Choose a reason for hiding this comment

yurloc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkobayas commented Apr 21, 2024 •

edited

Loading

Before PR in `drools-model-codegen`

After PR in `drools-model-codegen`

tkobayas Apr 21, 2024 •

edited

Loading

tkobayas Apr 22, 2024 •

edited

Loading

tkobayas Apr 21, 2024 •

edited

Loading

tkobayas Apr 22, 2024 •

edited

Loading

tkobayas Apr 22, 2024 •

edited

Loading

tkobayas Apr 22, 2024 •

edited

Loading

tkobayas Apr 22, 2024 •

edited

Loading